Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Datasets locking/v1 #12182

Closed
wants to merge 2 commits into from
Closed

Conversation

inashivb
Copy link
Member

In a recent warning reported by scan-build, datasets were found to be
using a blocking call in a critical section.

datasets.c:187:12: warning: Call to blocking function 'fgets' inside of critical section [unix.BlockInCriticalSection]
  187 |     while (fgets(line, (int)sizeof(line), fp) != NULL) {
      |            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
datasets.c:292:12: warning: Call to blocking function 'fgets' inside of critical section [unix.BlockInCriticalSection]
  292 |     while (fgets(line, (int)sizeof(line), fp) != NULL) {
      |            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
datasets.c:368:12: warning: Call to blocking function 'fgets' inside of critical section [unix.BlockInCriticalSection]
  368 |     while (fgets(line, (int)sizeof(line), fp) != NULL) {
      |            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
datasets.c:442:12: warning: Call to blocking function 'fgets' inside of critical section [unix.BlockInCriticalSection]
  442 |     while (fgets(line, (int)sizeof(line), fp) != NULL) {
      |            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
datasets.c:512:12: warning: Call to blocking function 'fgets' inside of critical section [unix.BlockInCriticalSection]
  512 |     while (fgets(line, (int)sizeof(line), fp) != NULL) {
      |            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
5 warnings generated.

Upon an analysis of the code, it was established that the call to all
the affected parts of the code is made by the fn DatasetGet which seems
to be only used by DetectDatasetSetup. A call to these Setup fns are
made by the rule loaders at the initialization of the detection engine
for a setup of expected keywords. Since this happens even before the
engine is started and there is just one thread active at the time, these
calls can be exempted from being marked critical section.

Dataset data is saved, accessed, looked up from the common THash API
when the actual traffic is matched against them.

Bug 7398
Copy link

codecov bot commented Nov 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 49.97%. Comparing base (4ec90bd) to head (25d4ecb).
Report is 16 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #12182      +/-   ##
==========================================
+ Coverage   49.75%   49.97%   +0.22%     
==========================================
  Files         912      912              
  Lines      257055   257048       -7     
==========================================
+ Hits       127895   128463     +568     
+ Misses     129160   128585     -575     
Flag Coverage Δ
fuzzcorpus 60.98% <ø> (-0.01%) ⬇️
livemode 19.60% <ø> (+0.18%) ⬆️
pcap 44.40% <ø> (?)
suricata-verify 62.69% <ø> (-0.02%) ⬇️
unittests 9.01% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@victorjulien
Copy link
Member

Since this happens even before the engine is started and there is just one thread active at the time, these
calls can be exempted from being marked critical section.

This is not correct. In multi-tenant setups there can be multiple loaders running in parallel.

@inashivb
Copy link
Member Author

Since this happens even before the engine is started and there is just one thread active at the time, these
calls can be exempted from being marked critical section.

This is not correct. In multi-tenant setups there can be multiple loaders running in parallel.

aha. Thank you!

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 23612

@inashivb inashivb closed this Dec 3, 2024
@inashivb inashivb deleted the datasets-locking/v1 branch December 3, 2024 13:01
@inashivb inashivb mentioned this pull request Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants